API review

Proposer: Ken Conley

Present at review:

• Tully Foote tfoote
• Jack O'Quin

To Review

sudo pip install rospkg

API Docs

Related: REP 114 rospkg standalone library

For now, please scope ignore the rospkg.distro API (except to comment on its inclusion). Also, the 'manifest' model is still being tweaked and generally considered an internal model for now.

API changes from roslib

1. Package API is now focused on RosPack instances instead of module-level functions like get_pkg_dir()

   • module-level functions required global cache to optimize performance, which make it hard to manipulate caching behavior. Now caching behavior is associated with each instance.
   • Overriding environment variables for module-level functions gets klunky

   • RosPack instance can be passed into higher-level libraries. For example, a rewritten rosdep can use one of these instances, which enables abstracting where package data is stored.

2. RosPack API has been rewritten from roslib equivalent to only accept single package arguments instead of list of packages arguments.

   • Multiple package style did not provide a performance enhancement
   • Multiple package style made unpacking return values klunky, e.g. "depends = rp.get_depends([package])[package]"

3. Standardizing on single "RosPack.get_depends(implicit=True)", "RosPack.get_rosdeps(implicit=True)" for API methods that enable querying explicit properties as well as implicit/recursive properties.

   • Previously, there was a mixture of "depend1", "rosdeps0", and other numerical means of specifying access.

   • Cleaner, simpler and more consistent API

4. RosPack and RosStack have identical APIs for same functionality (e.g. list()) provided by same implementation. Also, underlying Manifest representation has been unified so that same Manifest object can represent either a manifest.xml or stack.xml. For example, there is no longer a separate Depend and StackDepend class; there is just now a Depend object with an additional 'type' attribute set to 'stack' or 'package'.

5. os_detect now separates between "version" and "codename". These were previously mixed due to rosdep heritage. Now, higher-level libraries must make choice which to use

6. os_detect: check_presence() -> is_os(). New nomenclature to be more clear that there is no validation/side-effects, just a test of the detector.

7. OsDetect.register_default(). Constructor now gets default value from a class field of detectors. This enables dynamic registration of detectors instead of having to update OsDetect code directly.

8. Removed Mandriva support from os_detect. Does not appear to have been used and version return value was inconsistent with other similar OSes.

What's missing

• No load_manifest equivalent. Omitting this will limit the full deployment of Python infrastructure using just rospkg, but load_manifest is very tied to Python integration choices that may change over time.

• Anything to do with ROS communication system (e.g. environment does not provide for comm-related env variables)
• No support for ROS msgs/srvs: will be doing this in a separate standalone library that is decoupled from the packaging system.

Current usage

The rospkg API thus far has been exercised in a rosdep rewrite (RosPack and os detect APIs) as well as rewrites of the ROS release toolchain (mainly rospkg.distro).

The next step will be go through code in the ROS/ros_comm stacks and being porting from the equivalent roslib APIs. That step is awaiting this review.

Question / concerns / comments

Enter your thoughts on the API and any questions / concerns you have here. Please sign your name. Anything you want to address in the API review should be marked down here before the start of the meeting.

• kwc: should roscreate-pkg/stack be included as well?

  • tfoote: this would be nice to have, but might make sense to be released seperately into the pypi infrastructure, as it's only useful for people doing releases.

    • kwc: It's true that roscreate-stack is mainly for releases, but roscreate-pkg?

• mihelich: +1, no real concerns. The new API makes sense. I installed the pip version and played around in the interpreter without any big surprises. Some minor notes:

  • The docs list a RosStack.get_direct_depends() method, while the actual library uses RosStack.get_depends() with the implicit=True behavior, which is more consistent.

    • kwc: thanks for catching that. get_direct_depends() was old docs that I failed to remove from the sphinx.

  • Somehow the recursive get_rosdeps() failed for me:

    • kwc: thanks -- I hadn't written unit tests for that code path (I have now). The example works for me now.

>>> rospkg.RosPack().get_rosdeps('openni_camera', False)
[u'libusb1.0', u'ps-engine']
>>> rospkg.RosPack().get_rosdeps('openni_camera', True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python2.6/dist-packages/rospkg/rospack.py", line 289, in get_rosdeps
    return self._implicit_rosdeps(package)
  File "/usr/local/lib/python2.6/dist-packages/rospkg/rospack.py", line 309, in _implicit_rosdeps
    for p in pkgs:
NameError: global name 'pkgs' is not defined

tfoote

• The RosPack constructor could be simplified to just be a list of paths. It's more pythonic and the ROS_ROOT doesn't make a difference to finding packages, except that it should be there. Of course the would get rid of the ros_root methods and variables too. And this feature would need to be preserved at some higher level.

  • and same for RosStack

  • Another option might be an "RosEnv" class which contains the ROS_ROOT and ROS_PACKAGE_PATHs with storage and queries, and that class or RosEnv.list_paths() would be passed in as constructors.

• RosStack has a get_manifest method?

  • The RosEnv might make more sense as RosStack has the same environment methods too.

  • The RosEnv would also make a nice override for all the methods, instead of using the env parameterized input.

• I'd like to see get_ros_package_path return a list of paths, it's not specified, but I'm guessing it's just getting the string.
  • kwc: +1
• in os_detect Document as I assume that add_detector is the local only version of register_detector?
  • kwc: correct, one is a static method, the other affects the instance
• I don't know if this is in scope. On the rosdistro file format do we want to consider switching to using python substitution rules instead of $rules? Like %(STACK_NAME)s instead of $STACK_NAME and just use the python substitution rules?
  • kwc: a little out of scope... if we're going to change the rosdistro file format we should do a separate review.

kwc: based on your ideas, we could add a get_ros_path() as a replacement for get_ros_root() and get_ros_package_path(). get_ros_path() would return the ordered paths. I'll have to see if anything actually *needs* get_ros_root().

• ISSUE: the .rospack_cache requires knowledge of ROS_ROOT and ROS_PACKAGE_PATH. rospkg could not be deployed with this alternate API unless we update rospack to internally use this distinction as well.
• joq Looks good to me, +1.

PeterHousel:

• There is no way of invalidating the location cache if new stacks are checked out.
• The Manifest class does not appear in the generated API documentation, though objects of this type are accessible via get_manifest() methods.

• The get_manifest() method of rospkg.RosStack is documented as returning a package's manifest rather than a stack's manifest.

  • (kwc): thanks, fixed in 82:97ee51c3f8de

• The rosbuild2 tag is not yet documented in the Package manifest XML tags reference

  • (kwc): this tag is still under development

• There do not appear to be any unit tests for the unary stack (REP 109) case

  • (kwc): added, thanks

• Should there be a way of automatically retrieving a stack's version from CMakeLists.txt if it is not present in the stack manifest?

  • (kwc): Already exists: rosstack.get_stack_version().

• https://gist.github.com/1263777 contains a script written using this api intended (eventually) to generate OpenEmbedded recipes for cross-compilation of ROS stacks.

  • (kwc): cool, great to see rospkg/vcstools getting some exercise

Meeting agenda

To be filled out by proposer based on comments gathered during API review period

Todos

Additions:

• Implement RosPack.get_depends_on(implicit=True)

• Add rospkg.get_ros_paths() (see below)

• Added rospkg.get_package_name(path) (roslib.packages.get_dir_pkg() equivalent)

• DONE Document that RosPack RosStack are not thread-safe (due to caching)

• DONE Better documentation around Manifest, indicate that it is unstable

• DONE Added version number indicator

Changes:

• Change RosPack(ros_root=foo, ros_package_path=foo) to RosPack([paths])?

  • Corallary: RosStack

• Change RosPack.get_ros_root(), RosPack.get_ros_package_path() to RosPack.get_ros_paths()?

• Eliminate RosPack.ros_root and RosPack.ros_package_path properties?

Future work

• stabilize and review Manifest API

• stabilize and review distro API

Conclusion

Package status change mark change manifest)

• Action items that need to be taken.

• Major issues that need to be resolved